-
Notifications
You must be signed in to change notification settings - Fork 410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose the target element in INP attribution #478
Conversation
I think the element->target means the code examples would need updating. e.g. https://github.com/GoogleChrome/web-vitals?tab=readme-ov-file#send-attribution-data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this (once you address @Tiggerito 's concern about the README examples).
However, at risk of bikeshedding:
In the LCP entry it's known as element
not target
. For INP it is target
in the entry so I guess that's why we originally used it in the attribution object. I think the term "target" kinda makes sense to me for INP (and FID), but less so for LCP.
In fact the more I think about this, the more I think it's CLS that's wrong, not LCP. The shift wasn't "targeted"—in fact it was often the victim of another element being inserted. The LayoutShift entry has no target
entry (though it does have a sources.node
—maybe it should be largestShiftNode
).
However, saying all that, using "target" to mean the selector, does free up "element" to mean node for LCP so, for that reason, I think I'm in agreement with this PR even if it's a little inaccurate for LCP (and CLS).
I noticed we've not added it to FID either. Maybe that's OK since it's deprecated, but it doesn't seem like that much effort to add an eventTargetElement
.
Yeah, it's tricky because I think we want to balance three things:
And I'm honestly not sure what solution best address all three of these... That said, if this is a "pick two" kinda situation, I'd probably pick (1) and (3) since there is already inconsistency, so maybe it's better to just leave LCP and CLS as they are, and only introduce this for INP. Also, for LCP and CLS, there isn't actually a need for an element reference (yet), since the entries for those metrics will always contain a reference to the element. This is only needed for INP, since there are cases where Also, since we're already changing INP attribution names, we have more leway to pick the best name. And if we ever make a similar change for LCP and CLS (e.g. where we store a reference to the element ASAP to minimize null references), then we can similarly update those attribution interfaces. WDYT? If we just change INP, how about using these names? interface INPAttribution {
interactionTargetSelector: string;
interactionTargetElement: Node | undefined;
} |
That works for me! |
Updated. I'll also update the PR description once we finalize what changes we're making. |
cc: @just-jeb in case you have thoughts on this PR (either the naming, or in general). |
We discussed this offline and decided to close this in favor of #479, which keeps the original name the same. |
Fixes #456.
This PR exposes the
HTMLElement
object in addition to a selector pointing to that object, to support use cases where additional information from the element itself is useful.The following properties were added:
CLSAttribution.largestShiftTargetElement
INPAttribution.interactionTargetElement
LCPAttribution.targetElement
Breaking change:
In addition, since we realized we had inconsistent naming for LCP—where the
element
property was a CSS elector, whereas it's called "target" in every other attribution object—this PR renameselement
totarget
to be consistent:Given that this will be released with v4 and there is already a breaking change in the
LCPAttribution
interface, it made sense to rename rather than alias and deprecate.